Id-keyed TagMap.set(long) + insertion-comparison benchmark#11901
Draft
dougqh wants to merge 12 commits into
Draft
Id-keyed TagMap.set(long) + insertion-comparison benchmark#11901dougqh wants to merge 12 commits into
dougqh wants to merge 12 commits into
Conversation
Contributor
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
create(Object)/create(CharSequence) may return null for a null or empty value, and the Entry sinks -- getAndSet(Entry) / set(EntryReader) -- treat a null Entry as a no-op, so a null/empty value flows through the Entry pathway as "no tag" without any caller guarding. create(Object) now applies the empty-CharSequence check by runtime type, so the null/empty => absent convention holds regardless of the static type at the call site. The strict (key,value) setters keep their contract -- their values are now @nonnull -- so null tolerance is scoped to the Entry pathway. The annotations make the split self-describing. Fixes a latent NPE by construction: RemoteHostnameAdder sets create(TRACER_HOST, hostname) guarding only null, not empty, so an empty hostname previously NPE'd on set(null). Caller-side guard cleanup (incl. the redundant #11958 guard) is left to a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A tag has no valid null key, so put/set/getAndSet/create now take @nonnull tag. This completes the null contract alongside the value/Entry side: keys are strict (null = a bug), values/Entries on the Entry pathway are tolerant (null = no tag). Scoped to the write/create surface; read/lookup keys (getString, remove, getXxxOrDefault) are left for a possible follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TagMap was an interface with a single implementation, OptimizedTagMap. The split was vestigial scaffolding from when a second (HashMap-backed) impl existed; with one impl it is false generalization. Collapse them into one `public final class TagMap`: - The interface's abstract method declarations are removed; OptimizedTagMap's bodies become TagMap's methods. - Nested types that were implicitly `public static` in the interface (EntryChange, EntryRemoval, EntryReader, Entry, Ledger) are now written out explicitly as `public static`. - Static factories (create/fromMap/ledger/...) and the EMPTY constant become explicit `public static` members; the EmptyHolder lazy-init note is updated now that there is no interface<->impl class-init cycle. - putAll(TagMap) loses its `instanceof` dispatch (always true once there is one class) and calls the fast path directly. No behavior change; motivation is code simplicity, not performance (a single final class is monomorphic by construction, but CHA already devirtualized the sole impl). Public API is preserved, so callers are unchanged; the 3 tests that referenced OptimizedTagMap now reference TagMap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Post-fold tidy, all TagMap-scoped: - Remove EmptyHolder: with one class there is no interface<->impl class-init cycle to break, and the private constructor reads no statics, so EMPTY is a direct `new TagMap(new Object[1], 0)` initializer. - Static factories (create/fromMap/ledger/...) are now `public static final` (not expressible on the old interface). - assertSize/assertNotEmpty/assertEmpty/checkIntegrity test helpers dropped their now-always-true `instanceof TagMap` guard + redundant cast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A fresh, mutable TagMap can read through to a frozen parent on local misses, so a span can layer its own tags over a shared, immutable set (e.g. merged tracer tags) without copying them. - createFromParent(parent): the only way to attach a parent; the parent must be frozen and is fixed at construction (no re-parenting), so read-through can treat it as stable. Single-parent by design in phase 1. - Reads resolve local-first, then the parent; a local entry shadows the parent's (local-wins). Removing a parent key locally records a lazy tombstone (removedFromParent) so it stops reading through; the tombstone set is null until first needed, keeping the hot paths untouched. - size()/isEmpty() are exact (Map contract) and resolve the parent; isDefinitelyEmpty()/estimateSize() are the cheap conservative variants for the hot path. copy() preserves the parent and tombstones; forEach walks local then parent. Built on the folded final-class TagMap (#11967); composes cleanly with the null-tolerant Entry pathway (#11963). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dougqh
force-pushed
the
dougqh/dense-bloom-filter
branch
from
July 15, 2026 20:44
428e72f to
267ebad
Compare
…plit phase 1) Attach the trace's merged tracer tags to each span's TagMap as a frozen read-through parent (via TagMap.createFromParent) at span construction, instead of copying them into every span. The span sees the shared tags on read and only stores its own local tags, so the common trace-level bundle is held once per trace rather than duplicated per span. - CoreTracer builds the frozen merged-tracer-tags parent once; config version is kept out of that bundle. - DDSpanContext attaches the parent at construction (fixed, no re-parenting). - Adds TagMapReadThroughBenchmark (copy-down vs read-through, -prof gc). Stacked on the read-through mechanism (#11789), which builds on the folded final-class TagMap (#11967). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
StringIndex is a compact open-addressed string→index structure (the keyOf substrate the dense tag store builds on): parallel hash/name arrays, linear probing, on par with HashSet on lookup at a smaller footprint. Includes unit tests, a footprint test (jol), and comparison benchmarks (vs HashSet/switch). No TagMap changes — standalone util. Rebased onto the level-split stack (consumer #11932) as the layer dense-store sits on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Known tags (keyOf resolves to a stored id) are held in insertion-ordered parallel arrays (knownIds/knownValues) with NO per-tag Entry object — the allocation lever. Lazily allocated on the first known-tag write; custom tags stay in the hash buckets. Disjoint by construction (known-ness is global), so read-through shadow checks stay within-region and the bucket path is unchanged. - KnownTagCodec (id encoding + resolver) + hand-written KnownTags (keyOf substrate over StringIndex). Off-by-default: dormant until a resolver registers, so production is byte-identical. - CoreTracer flips it live behind `-Ddd.trace.dense.tags.enabled` for A/B. - Sizing is a generous fixed stopgap (KNOWN_INIT_CAP=12, the per-type max); exact per-type sizing comes with the tag registry. Reconciled onto the level-split stack (fold + read-through + StringIndex); built on the folded final-class TagMap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…scan) A per-map long bitmask (knownBloom) over the dense store: a set bit means a tagId MAY be present (scan to confirm), a clear bit means DEFINITELY absent — so the common per-build insert skips the linear knownIndexOf scan and appends in O(1). Crude position->bit map (fieldPos & 63); a collision-minimizing per-type coloring later only raises the hit rate — correctness never depends on it because the scan stays authoritative. Superset semantics: set on add, never cleared on remove (a stale bit costs a scan, never a wrong answer). Alloc-neutral (one long field, no extra allocation); the win is insertion CPU, moving the dense store toward HashMap insertion parity without the scan. Reconciled onto the folded-class dense store (#11814). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dougqh
force-pushed
the
dougqh/dense-bloom-filter
branch
from
July 15, 2026 21:23
267ebad to
56550cb
Compare
Replace the hand-written KnownTags with a build-time code generator: a language-agnostic tag-conventions.yaml (span types via extends/include/applies) + a Java overlay (tag-conventions.java.yaml: intercepted hints + the virtual registry) drive a buildSrc generator that emits KnownTags.java (id constants + keyOf/nameOf resolver) to committed src/generated. A verifyKnownTags check regenerates and byte-compares to catch stale commits; src/generated is excluded from spotless (the emitter produces google-java-format-clean output). - buildSrc: TagRegistryGeneratorPlugin + generate/verify tasks + parse/resolve (TagConventions) + id-assignment/coloring (TagRegistry) + emit (KnownTagsEmitter). - KnownTagCodec gains the name-free 3-arg tagId(globalSerial, intercepted, slot) the generator uses. - Generated per-type layout reports (resolved/tag-assignment/layout-by-type/ folded-types) committed as auditable artifacts — the per-type counts are the substrate the (follow-on) per-type dense-array sizing reads. Substrate only: this lands the generated ids/resolver + the codegen source of truth; it does NOT yet change dense-array sizing (that per-type-sizing step is the follow-on that climbs out of the alloc valley). Reconciled onto the dense + bloom stack; generated ids cohere with the dense store (forked/fuzz green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
set(long id, Object value) is the id-keyed insertion path: the caller passes a resolved KnownTags id, so it skips the keyOf name resolution the set(String, ...) methods pay and stores densely. The id must be a stored known-tag id; custom names have no id and use the name-keyed setters. The name is resolved lazily only to clear a read-through tombstone (rare). Adds TagMapInsertionComparisonBenchmark (dense vs HashMap insertion, -prof gc) — the isolated micro where the dense store + bloom insertion win shows, and the home for the id-keyed vs string-keyed comparison. Reconciled onto the tag registry (#11961). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dougqh
changed the base branch from
dougqh/dense-bloom-filter
to
dougqh/tag-registry-generator
July 15, 2026 21:27
dougqh
force-pushed
the
dougqh/tag-id-api
branch
from
July 15, 2026 21:27
f0cb351 to
7e3ed31
Compare
|
🎯 Code Coverage (details) 🔗 Commit SHA: 7e3ed31 | Docs | Datadog PR Page | Give us feedback! |
dougqh
force-pushed
the
dougqh/tag-registry-generator
branch
from
July 22, 2026 16:32
ad3c1f7 to
369a591
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
On top of the bloom fast-path (#11900):
TagMap.set(long id, Object)— the id-keyed insertion path: given a resolvedKnownTags.*_ID, store densely and skipkeyOfname resolution. SoleTagMapimpl (OptimizedTagMap); the id must be a stored known id (custom names keep the name setters).TagMapInsertionComparisonBenchmark— HashMap / 1.0-bucket / 2.0-name / 2.0-id, alloc + throughput; the numbers behind the deck's slides 3/7/8.Results (idle box,
-prof gc -f 5 -wi 5 -i 5; persisted in the benchmark header)hashMaptagMapByIdtagMapByNametagMapCustom(1.0 bucket)id==name(the bloom/dense win is CPU, not alloc).fieldPos & 63.Scope
Minimal id API (TagMap level). Full
AgentSpan.setTag(long)+ theKnownTagscall-site migration are follow-ons.set(long)has no production caller yet — by design, it lands with its benchmark evidence ahead of the migration that consumes it (each PR stands alone in sequence).Stacking
#11814 (dense storage) → #11900 (bloom) → this (id API + comparison benchmark).
Follow-ups
TagMap.Ledger+SpanBuilderid support; per-type graph coloring; master run to pin true-1.0 (nokeyOf).🤖 Generated with Claude Code